Improve cover z-index solution #66249

renatho · 2024-10-18T15:29:49Z

What?

This is an improved solution for this original PR: #66093

As part of this PR, I also thought of removing this effect that adds the has-modal-open in the editor because that wouldn't be needed for this purpose anymore. But thinking more, I kept it to keep the consistency there between the frontend and the editor.

Why?

To improve the solution and eliminate the dependencies between Cover and Navigation blocks (the class that Navigation adds being used in the Cover styles).

More context starting from this comment in the thread.

How?

It deprecates the current version of the Cover block, changing the order of the elements, and removing the z-index of them.

It continues using the has-modal-open class for backward compatibility (solution introduced in #66093), but when the code is updated it's not used anymore (after the save).

Testing Instructions

Before switching to this branch, add a Cover block to a post.
Inside the cover block, add a Navigation block.
After the previously created Cover block, add another Cover block.
Switch to this branch.
Visit the site in the frontend on small screens and also visit the editor. Open the menu, and make sure it's displayed over the whole content.
Edit something in the post, and save to update the deprecated version of the block.
Check that the frontend continues working properly.

Screenshots or screencast

Screen.Recording.2024-10-14.at.11.28.51.mp4

Since I continued seeing the same result in my tests, I reused the same video from #66093 just to illustrate the tests.

renatho · 2024-10-18T15:31:43Z

packages/base-styles/_z-index.scss

 	".wp-block-cover.has-background-dim::before": 1, // Overlay area inside block cover need to be higher than the video background.
-	".block-library-cover__padding-visualizer": 2, // BoxControl visualizer needs to be +1 higher than .wp-block-cover.has-background-dim::before


I'm removing it again because the change discussed here was reverted.

renatho · 2024-10-18T15:33:17Z

packages/block-library/src/cover/save.js

@@ -88,7 +88,8 @@ export default function save( { attributes } ) {
 			'has-custom-content-position':
 				! isContentPositionCenter( contentPosition ),
 		},
-		getPositionClassName( contentPosition )
+		getPositionClassName( contentPosition ),
+		'has-no-zindex-in-inner-blocks'


I didn't like this name, but I couldn't find a better one. I though about a has-natural-elements-order or something like this, but the has-no-zindex-in-inner-blocks seems more self-explanatory about the purpose of this.

Could we avoid the introduction of the CSS class entirely using sibling CSS selectors?

This would also mean that only blocks with both an overlay and media background would need the reordered markup, reducing the number of blocks requiring a migration via the deprecation.

Please, let me know if I'm missing something from your suggestion, but I think we still need it.

The main goal of our change is the z-index cleanup, removing especially the z-index from the .wp-block-cover__inner-container. But we can do it only with the new markup, otherwise, it would break styles for those that don't re-save their posts.

In other words, the has-no-zindex-in-inner-blocks class is more like a versioning class (with a more semantic name than a vXX) to check if we have the new markup to apply the new styles.

removing especially the z-index from the .wp-block-cover__inner-container. But we can do it only with the new markup, otherwise, it would break styles for those that don't re-save their posts.

The container could have a style using :has() that checks the order of direct children using the + sibling selector. If they are in the old order, the style applies the z-index for BC. If the children are in the new order, no z-index would be applied. Another option in our toolkit would be to use ::first-child when checking the order etc.

As noted in my other comment, I might not have the bandwidth to give this a try until later this week or early next. Hopefully, this makes sense and gives you a thread to follow though 🧵

Yes! I could iterate and answered in the other thread. 😉 Thank you for your suggestions!
#66249 (comment)

renatho · 2024-10-18T16:38:29Z

packages/block-library/src/cover/style.scss

-			z-index: auto;
+	&.has-no-zindex-in-inner-blocks {
+		.wp-block-cover__inner-container {
+			position: relative; // It needs a position to be on top of the other elements.


Please, look at this property with a grain of salt. I think it won't cause any issue, but if we had an element inside this block with a position absolute, it would be affected.

The next block in the tree with the position relative would be the immediate parent (.wp-block-cover). I don't believe we would have an existing think trusting in the position relative to the .wp-block-cover, but we never know! =P

After a quick glance it seems okay. Once the deprecation and styles are iterated on we can test this aspect after they've settled.

github-actions · 2024-10-18T16:41:53Z

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core, Gutenberg Plugin.
Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

github-actions · 2024-10-18T16:41:54Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: renatho <renathoc@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

renatho · 2024-10-18T16:42:35Z

I'm already opening it for review, but it still has some test issues.

I tried to update the fixtures through npm run fixtures:regenerate, but I received some errors. If someone could explain me what is happening and how I could fix, it would be very helpful to save some time. =)

aaronrobertshaw

Thank you for following up on #66093 @renatho 👍

I've taken this for a test drive. It's coming together well but I did run into a few issues.

✅ Cover z-index behaviour works as expected post migration
✅ Deprecated blocks appear to migrate in the editor
✅ Deprecated blocks still render appropriately on the frontend
❌ After saving the post and reloading, the deprecated blocks don't appear to have been migrated i.e. they retrigger the deprecation. Making an unrelated edit and resaving the post seems to apply the updates the second time around.

Screen.Recording.2024-10-21.at.3.06.58.pm.mp4

In addition to the migration issue flagged above, I've left a few other thoughts as inline comments where applicable.

These include:

We should be able to avoid introducing the new CSS class to the markup by using CSS sibling selectors for both the latest and BC styles.
- This would also mean that less blocks would need to be migrated, perhaps mitigating a tiny bit of the performance hit.
- i.e. only blocks with both the color overlay and an image or video would have their markup changed requiring a migration.
As the original fix is slated for release in 19.6, we could avoid the need to include styles for .has-modal-open for BC reasons.
- I understand the consistency argument but simply including it for no functional reason and setting a precedent for a block in the editor to mess with the document root classes feels like a stretch.
- Happy to hear others' thoughts on that one. My vote is remove it, we can always add it when there is a pressing need.
We need a new fixture to cover this latest deprecation.
All the non-deprecation-related fixtures for the cover block need to be updated to the latest markup structure.
- This is why the fixture regeneration is throwing the error around an unexpected deprecation being run.
After the above fixture updates, regenerating the fixtures should succeed and the result should be something like past deprecation's serialized HTML being updated to the correct structure.

I suspect that in the process of ironing out the deprecation, the bug I noticed while testing will be uncovered and resolved 🤞

@renatho let me know if anything around my recommendations for the deprecation tweaks isn't clear.

aaronrobertshaw · 2024-10-21T05:46:06Z

packages/block-library/src/cover/editor.scss

+.wp-block-cover:not(.has-no-zindex-in-inner-blocks) {	// Shown while media is being uploaded
+	.components-spinner {
+		z-index: z-index(".wp-block-cover__inner-container");
+	}
+}


Do we need to scope this by the proposed CSS class? The old spinner style would result in the same z-index as the inner container, which comes later in the DOM.

At a quick glance, the positioning of the spinner before the inner container is the same on trunk and this PR. In both cases the spinner is rendered beneath the inner container.

Do we need to scope this by the proposed CSS class?

We could keep it as it was. I changed it more as part of the z-index cleanup.
I don't have a strong opinion about this element, so if you have, let me know that I keep this z-index.

At a quick glance, the positioning of the spinner before the inner container is the same on trunk and this PR. In both cases the spinner is rendered beneath the inner container.

Just notice that if we keep the z-index in the spinner and remove from the inner blocks (where it's important to fix the issue), it would change the original behavior that you described. I didn't test, but I think it would probably be better because I suspect we would always want the spinner in the front.

I'm not following that last suggestion sorry. Could you clarify it for me?

If you're suggesting that we change the behaviour of the spinner to bring it to the top, I'm not so sure about that. I'd vote that we don't introduce more changes here that aren't required for the bug fix. We should keep trunk's behaviour then make an informed decision on bringing the spinner forward, probably with some design feedback too.

Could you clarify it for me?

Thinking more now, that .wp-block-cover:not(.has-no-zindex-in-inner-blocks) probably wouldn't be needed because it is in the editor side and it would always be with the latest HTML. But we needed to remove the z-index from the spinner because the inner container doesn't have the z-index anymore, so the spinner would be above it (I suppose it's expected to have the inner container above, like in the trunk, so the user can edit the content while the media is being uploaded).

Here are 2 screenshots just for more clarification:

That said, I tweaked it after applying the new discussed approach: e29b238

We needed to add a z-index in the inner container during the transient state (uploading) because of the transient overlay. It means that we would have the original issue (the cover appearing over the navigation overlay) in this moment, but I suspect it's fine since it's just this quick moment and it's only in the editor side. WDYT? If you consider it bad, we could try to explore other approaches to change the transient overlay, but I don't think it's worth it.

aaronrobertshaw · 2024-10-21T05:47:45Z

packages/block-library/src/cover/save.js

@@ -88,7 +88,8 @@ export default function save( { attributes } ) {
 			'has-custom-content-position':
 				! isContentPositionCenter( contentPosition ),
 		},
-		getPositionClassName( contentPosition )
+		getPositionClassName( contentPosition ),
+		'has-no-zindex-in-inner-blocks'


Could we avoid the introduction of the CSS class entirely using sibling CSS selectors?

This would also mean that only blocks with both an overlay and media background would need the reordered markup, reducing the number of blocks requiring a migration via the deprecation.

aaronrobertshaw · 2024-10-21T05:49:14Z

packages/block-library/src/cover/style.scss

@@ -302,3 +292,32 @@ section.wp-block-cover-image > h2,
 		color: inherit;
 	}
 }
+
+// Backward compatibility for v14 deprecation.
+.wp-block-cover:not(.has-no-zindex-in-inner-blocks) {


As noted in the above thread, it could be possible to avoiding needing .has-no-zindex-in-inner-blocks completely.

aaronrobertshaw · 2024-10-21T05:51:39Z

packages/block-library/src/cover/style.scss

+		// Reset the z-index to auto when the body has a modal open. So when the
+		// modal is inside the cover, it doesn't create a stacking context.
+		@at-root .has-modal-open & {
+			z-index: auto;
+		}


If #66093 isn't slated for release until 19.6, could we also avoid these additional styles? I don't think we need to provide BC for a change that hasn't been released yet.

I think it's good to have this anyway, so it's automatically fixed in all the sites just by updating the Gutenberg. Make sense?

Maybe my memory is failing me but the original issue was in the editor only correct? The interactivity API was used on the frontend and that was all displaying ok.

In the editor, with a deprecation in place, the blocks would be migrated and thus fixed without needing the superfluous styles. So the problem would still be automatically fixed by updating Gutenberg only without the unnecessary styles and the block modifying document level classes.

The main issue was on the frontend (we had a few reports about that from users). And as a lower impact, it was also happening in the editor.

Some reports from users can be found in:

Navigation Block: Overlay Z-index nesting issues with consecutive cover blocks Automattic/wp-calypso#75439

Navigation block: overlapping(z-index) issue when nested inside Cover Block Automattic/wp-calypso#79317

Thanks for setting me straight on that.

So, we can leave this style there but the has-modal-open class being added to the document wrapper should be removed. The deprecation should sort out the migration and display of the block in the editor so we no longer need the change that was introduced in #66093.

Avoiding that was the main driver for this whole approach in my view.

I thought the same, but I decided to keep given this reason:

As part of this PR, I also thought of removing this effect that adds the has-modal-open in the editor because that wouldn't be needed for this purpose anymore. But thinking more, I kept it to keep the consistency there between the frontend and the editor.

If you reconsider, and think it's still worth removing it, I can remove it. But I think it would be better to keep this for consistency (frontend - editor) for the future.

Avoiding that was the main driver for this whole approach in my view.

I think the benefit is that we only use it for BC and not as our main solution anymore.

aaronrobertshaw · 2024-10-21T05:52:57Z

packages/block-library/src/cover/deprecated.js

@@ -244,6 +244,157 @@ const v12BlockSupports = {
 	},
 };

+// Deprecation for blocks that have z-index.
+const v14 = {


This deprecation will need a fixture to cover it. Here's an example for an earlier deprecation.

Thank you for the suggestion and the example! ♥️
I'll try to fix it today!

I followed this documentation, and based on your example, I added the new fixture: 9ba72c0 (created the HTML and let the command generate the others)

I'm still struggling a bit to fix the others. I let you know here when it's done.

I fixed most of the issues (0ae6273), but in the fixture tests it's trying to add the 'has-no-zindex-in-inner-blocks' as a className attribute.

Not sure if it's a bug with the fixture tests, or if adding a class to the wrapper element wouldn't be recommended. 🤔

I thought of not using the sibling class, as you suggested to fix this, but it would mean a few new classes instead of just one. Another approach would be adding an additional wrapper with a class, it would break a few styles (we have some using > that we would need to refactor to do it, so I think it would be risky).

I'm out of ideas now. I need to think more.

And I noticed that I also need to update some mobile snapshots.

Thanks for the updates, I'll review those shortly.

Regarding the sibling selectors though, why would we need additional classes?

We should be able to rely on the order of the elements to have the correct sibling selector apply because the old and new versions moved the overlay element. When there aren't multiple elements there, we wouldn't need to juggle their z-index.

I'm a little short on bandwidth at the moment to code something up but I could take a run at it later this week perhaps if you don't beat me to it.

It could be worth pursuing this, even if it is ruled out as an option, as it would limit the number of blocks needing migration and avoid the slightly awkward class on the wrapper.

Hey! 👋

Thank you for your suggestions! I followed your suggestion and applied it with this commit: 0e25fed.

So we don't need to set z-index in all the elements, creating the stacking context for the inner blocks, for example.

renatho · 2024-10-22T21:39:14Z

❌ After saving the post and reloading, the deprecated blocks don't appear to have been migrated i.e. they retrigger the deprecation. Making an unrelated edit and resaving the post seems to apply the updates the second time around.

@aaronrobertshaw, could this be related to the way it works when using the code editor mode? I could reproduce the same, but it seems it doesn't change the blocks when we are in the code editor mode. If we are in the visual mode and we edit anything and save, it changes properly, even if it's on the first try.

aaronrobertshaw · 2024-10-23T07:23:45Z

could this be related to the way it works when using the code editor mode?

Good point. I'll give it a bit more of a test. We'll also need to ensure that it is migrated correctly if patterns containing a deprecated version of the cover block are inserted.

aaronrobertshaw

Thanks for updating the deprecation fixtures @renatho 💪

That's a lot of files! 😅

Unfortunately, it looks like a large number shouldn't have needed changes.

All the non-deprecation-related fixtures for the cover block need to be updated to the latest markup structure

I'm sorry I wasn't clearer with this comment in my last review.

All the source fixtures for deprecations e.g. *_deprecated-#.html should remain as before. These fixtures are supposed to cover the deprecated versions of the block, as the were when they were deprecated.

Only the source fixtures for the current version of the block needed to be updated to the latest markup structure. These are all the .html files that don't have __deprecated-# in their name.

After the above fixture updates, regenerating the fixtures should succeed and the result should be something like past deprecation's serialized HTML being updated to the correct structure.

With the source fixtures being correct, regenerating the fixtures would mean that the _deprecated-#.serialized.html files all get updated showing they successfully migrate to the latest version i.e. with the wrapper's class (if it's kept) and the reordered inner elements as appropriate.

It might not seem like it but I don't think this PR is that far off. The deprecation itself seems ok, we just need to sort out the fixtures and the CSS approach.

I'd recommend to iterate on the CSS first as the possible removal of the need for a new CSS class would reduce the number of deprecation fixtures requiring change. Doing it the other way around would likely create unnecessary rework.

Thanks again for all your hard work and patience here @renatho 🙇

renatho · 2024-10-23T14:34:17Z

Thank you for all the quick and priceless reviews, @aaronrobertshaw! And thanks for your patience! =)

All the source fixtures for deprecations e.g. *_deprecated-#.html should remain as before. These fixtures are supposed to cover the deprecated versions of the block, as the were when they were deprecated.

Only the source fixtures for the current version of the block needed to be updated to the latest markup structure. These are all the .html files that don't have __deprecated-# in their name.

Thanks! That makes more sense now! I'll wait to fix them after we have the rest done. :)

renatho commented Oct 18, 2024

View reviewed changes

renatho force-pushed the fix/improve-cover-zindex-solution branch from cccb26d to e9c5f0b Compare October 18, 2024 15:57

This was referenced Oct 18, 2024

Error on multiple clicks in the navigation modal when having a cover block after this #66066

Open

Fix the navigation issue inside cover blocks #66093

Merged

renatho commented Oct 18, 2024

View reviewed changes

renatho marked this pull request as ready for review October 18, 2024 16:41

renatho requested a review from ajitbohra as a code owner October 18, 2024 16:41

aaronrobertshaw reviewed Oct 21, 2024

View reviewed changes

renatho force-pushed the fix/improve-cover-zindex-solution branch 3 times, most recently from 8835572 to a8af387 Compare October 22, 2024 19:35

renatho added 7 commits October 22, 2024 18:15

Remove not used variable

441c4e1

Change the order of the elements to the expected layers order

1c28378

So we don't need to set z-index in all the elements, creating the stacking context for the inner blocks, for example.

Update z-index styles to be applied only for backward compatibility

233b4ae

Add position relative to keep inner content on the top

0e736bd

Fix spinner position to be over the overlay

a9fe8da

Add new fixture

4f01593

Update cover fixtures

0ae6273

renatho force-pushed the fix/improve-cover-zindex-solution branch from a8af387 to 0ae6273 Compare October 22, 2024 21:17

Update deprecation to match last update from trunk

4b67126

renatho force-pushed the fix/improve-cover-zindex-solution branch from bbcf05a to 4b67126 Compare October 22, 2024 21:30

Add class to the editor

3369f0f

aaronrobertshaw reviewed Oct 23, 2024

View reviewed changes

renatho force-pushed the fix/improve-cover-zindex-solution branch from c108205 to 3369f0f Compare October 23, 2024 12:15

renatho added 3 commits October 23, 2024 10:02

Remove class in favor of a new approach checking the children

0e25fed

Use styles only on frontend for backward compatibility

9a6d996

Fix loading state

e29b238

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cover z-index solution #66249

Improve cover z-index solution #66249

renatho commented Oct 18, 2024 •

edited

Loading

renatho Oct 18, 2024

renatho Oct 18, 2024

aaronrobertshaw Oct 21, 2024

renatho Oct 21, 2024

aaronrobertshaw Oct 23, 2024

renatho Oct 23, 2024

renatho Oct 18, 2024 •

edited

Loading

aaronrobertshaw Oct 21, 2024

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024 •

edited

Loading

renatho commented Oct 18, 2024

aaronrobertshaw left a comment

aaronrobertshaw Oct 21, 2024

renatho Oct 22, 2024

aaronrobertshaw Oct 23, 2024

renatho Oct 23, 2024 •

edited

Loading

aaronrobertshaw Oct 21, 2024

aaronrobertshaw Oct 21, 2024

aaronrobertshaw Oct 21, 2024

renatho Oct 21, 2024

aaronrobertshaw Oct 22, 2024

renatho Oct 22, 2024 •

edited

Loading

aaronrobertshaw Oct 23, 2024

renatho Oct 23, 2024

aaronrobertshaw Oct 21, 2024

renatho Oct 22, 2024

renatho Oct 22, 2024

renatho Oct 22, 2024 •

edited

Loading

aaronrobertshaw Oct 23, 2024

renatho Oct 23, 2024

renatho commented Oct 22, 2024

aaronrobertshaw commented Oct 23, 2024

aaronrobertshaw left a comment

renatho commented Oct 23, 2024

		".wp-block-cover.has-background-dim::before": 1, // Overlay area inside block cover need to be higher than the video background.
		".block-library-cover__padding-visualizer": 2, // BoxControl visualizer needs to be +1 higher than .wp-block-cover.has-background-dim::before

Improve cover z-index solution #66249

Are you sure you want to change the base?

Improve cover z-index solution #66249

Conversation

renatho commented Oct 18, 2024 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024 • edited Loading

renatho commented Oct 18, 2024

aaronrobertshaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

renatho commented Oct 22, 2024

aaronrobertshaw commented Oct 23, 2024

aaronrobertshaw left a comment

Choose a reason for hiding this comment

renatho commented Oct 23, 2024

renatho commented Oct 18, 2024 •

edited

Loading

renatho Oct 18, 2024 •

edited

Loading

github-actions bot commented Oct 18, 2024 •

edited

Loading

renatho Oct 23, 2024 •

edited

Loading

renatho Oct 22, 2024 •

edited

Loading

renatho Oct 22, 2024 •

edited

Loading